-
Notifications
You must be signed in to change notification settings - Fork 3.4k
chore: migrate @packages/extension to TypeScript and tests to Vitest
#32680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: migrate @packages/extension to TypeScript and tests to Vitest
#32680
Conversation
cypress
|
||||||||||||||||||||||||||||||||||||||||
| Project |
cypress
|
| Branch Review |
chore/extension_migration
|
| Run status |
|
| Run duration | 19m 07s |
| Commit |
|
| Committer | Bill Glesias |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
13
|
|
|
1102
|
|
|
4
|
|
|
26730
|
| View all changes introduced in this branch ↗︎ | |
Warning
Partial Report: The results for the Application Quality reports may be incomplete.
UI Coverage
45.56%
|
|
|---|---|
|
|
185
|
|
|
159
|
Accessibility
97.97%
|
|
|---|---|
|
|
4 critical
8 serious
2 moderate
2 minor
|
|
|
101
|
6772100 to
a3e39ab
Compare
a3e39ab to
8b308cd
Compare
8b308cd to
c7c22b2
Compare
c7c22b2 to
6a699b9
Compare
6a699b9 to
7a1c3de
Compare
| @@ -1,10 +0,0 @@ | |||
| module.exports = { | |||
| getCookieUrl: (cookie = {}) => { | |||
| const prefix = cookie.secure ? 'https://' : 'http://' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved into the automation cookie code in the server as it is the only place its used
7a1c3de to
73dac30
Compare
@packages/extension to TypeScript and tests to Vitest
73dac30 to
64d76a8
Compare
64d76a8 to
de94f36
Compare
de94f36 to
6ecc170
Compare
6ecc170 to
476783e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of tests that just assert some function to not be called, but no other positive assertion in the test - like these tests would also pass if we just broke the implementation. I know it's just transferring what we currently have, but that made me a bit nervous during review.
|
|
||
| return prefix + host + (cookie.path || '') | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AtofStryker I know this is not meant to be a refactor, but this note from CursorBot is interesting.
@jennifer-shehane can you elaborate on which positive assertion calls are missing? I feel like we have pretty good coverage on the v2 background and v3 service-worker/content code where we are asserting the expected positive condition, but in some cases we are testing a lot of no-op scenarios in addition to these. At least at a glance things looks well tested Eventually we can start getting some v8 code coverage reports as well once we get closer to having some type of test unification |
|
@AtofStryker Yah, it's a lot of noop situations basically is what I was pointing out. |
@jennifer-shehane I think thats fair. It's a bit bizarre but understandable because there is really one happy path and 4-5 other not happy paths so there are a lot of unit tests for the not happy path |
50ceb5a
into
chore/refactor-socket-and-extension

Additional details
With the changes to
@packages/socket,@packages/extensionneeds to be able to have a test environment that better supports ESM. To do that, this PR migrates@packages/extensiontoTypeScriptandVitest. The extension is now bundled slightly different than before. This is now lined out in the README.md:@packages/extensionhas a few different build processes occurring that are all driven by thegulpfile.app- The web extension piece of the code, has two separate bundles:v2: Version 2 of the web extension which uses webpack to bundle theapp/v2directory and output it asbackground.jsv3: Version 3 of the web extension, which doesn't have any external dependencies so we are able to compile down to `ESM to run in the browser natively.lib- the@packages/extensionmainentry that has utility methods on how to find/load the extension. This is transpiled to CommonJS as it is consumed in the Node context.The
libsection now ships with declarations compiled since the move to TypeScript, hence why we awere able to remove the old declarations and entry.Outside of this, I moved a few files around as we are still dealing with circular dependencies.
errors.cloneinside@packages/proxy. before it was referencing the server and creating a circular reference (cookie automation circular references still remain)@packages/extensiondependency from the automation cookie logic and move thegetCookieUrlfunction into the automation client as it isn't used anywhere else. Eventually, the cookie automation logic will be extracted out of the server, likely to network-tools or something adjacent.Steps to test
Everything should behave the same. If running locally, you likely want to run a
git clean -fxdand reinstall dependencies to see the appropriate build folders. Follow the steps to debug the extension in the readmeHow has the user experience changed?
PR Tasks
cypress-documentation?type definitions?Note
Migrates @packages/extension to TS/ESM with Vitest, introduces new app/lib build outputs, updates webpack/gulp, replaces legacy tests, and adjusts server/proxy imports and cookie URL logic.
app/v2andapp/v3code to TypeScript/ESM; replace CommonJS requires; add typings for webextension APIs.app-dist/(extension) andlib-dist/(Node lib); update paths in helpers; add.gitignorefor these.init.ts; addwebpack.config.mjs(TS-loader w/tsconfig.app.v2.json).gulpfile.ts: switch to buildingv2/v3/libvia yarn scripts; copy assets toapp-dist; get icons/logos from@packages/icons.package.json:main->lib-dist/index.js, add build scripts (build:v2,build:v3,build:lib), switch tests tovitest, add deps (vitest,webpack-cli,@types/webextension-polyfill).index.js,index.d.ts,lib/extension.js,lib/util.js); addlib/index.tsreplacement and TS configs (tsconfig.*).*.spec.ts), delete legacy JS specs and mocha setup; addvitest.config.ts.app/v2/background.ts,client.ts,init.ts: typed APIs, importBluebird, exportautomation.app/v3/content.tsandservice-worker.ts: add TypeScript annotations and stricter message handling.server/lib/automation/cookies.tsand use there; drop dependency on@packages/extensionfor cookies.* as extensionfor path helpers.@packages/errors.expectedResultCountfrom5to4in unit-tests step.packages/extensionas completed in ESM migration checklist.dist/toapp-dist/.Written by Cursor Bugbot for commit 30bd3fc. This will update automatically on new commits. Configure here.